Implement image builder database maintenance (HMS-4244) #1402

schuellerf · 2024-11-14T12:58:26Z

This implements the retention policy requested in https://issues.redhat.com/browse/HMS-4244

image-builder-maintenance for now runs vacuum
and cleans all composes older than 2 years (by default)
to remove customer related data according to the
retention policy

As this is not really urgent, it's scheduled once
a week on Tuesdays to avoid problems on weekends

…should be merged after #1398
Note: this implementation is almost a copy of https://github.com/osbuild/osbuild-composer/tree/main/cmd/osbuild-service-maintenance to stick to the same mechanism

codecov · 2024-11-14T13:03:52Z

Codecov Report

Attention: Patch coverage is 91.89189% with 3 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
cmd/image-builder/main.go	0.00%	3 Missing ⚠️

Files with missing lines	Coverage Δ
internal/db/db.go	`57.81% <100.00%> (ø)`
internal/tutils/psql.go	`75.43% <100.00%> (+5.54%)`	⬆️
cmd/image-builder/main.go	`0.00% <0.00%> (ø)`

schuellerf · 2024-11-15T14:01:17Z

Note: failed code/snyk check just indicates that in our testcases we have a password hardcoded …

lzap

Looks great. The only concern I have is why this is a separate image, but this could be app-sre requirement, no issues.

cmd/image-builder-maintenance/config.go

cmd/image-builder-maintenance/db.go

Makefile

build_deploy.sh

cmd/image-builder-maintenance/main.go

schuellerf · 2024-11-15T15:23:02Z

@lzap
I forgot to add this to the commit message that this, for now, is almost a copy of
https://github.com/osbuild/osbuild-composer/tree/main/cmd/osbuild-service-maintenance
to stick to the same approach.

schuellerf · 2024-11-15T15:27:37Z

Refactoring this into a separate repo or somehow sharing code with composer was considered but discarded for less effort.
Still this discussion is valid and ongoing.

schuellerf · 2024-11-19T10:00:25Z

Konflux failure seems to be related to the current rhsm outage… (and it's not a required check here)
… and PR build seems to fail due to ephemeral cluster issues

schuellerf · 2024-11-19T10:29:56Z

/retest

schuellerf · 2024-11-19T16:29:53Z

/retest

lzap

Couple of logging/transactional improvements could be done, but nothing big. I mean, there are not too many records, are there? :-)

cmd/image-builder-maintenance/db.go

templates/maintenance.yml

cmd/image-builder-maintenance/main.go

mvo5

Quick drive-by: when switching to "cleanenv", please split the commit that changes our code and the part that adds the vendoring to the repo (i.e. two commits, one with vendored stuff, one with switching the code to use it)

Also do we actually need to put the vendored deps into the git repo? We removed all vendoring from "images" because it is not packaged, maybe we could look into doing the same here?

schuellerf · 2024-11-22T07:53:03Z

@mvo5

Quick drive-by: when switching to "cleanenv", please split the commit that changes our code and the part that adds the vendoring to the repo (i.e. two commits, one with vendored stuff, one with switching the code to use it)

I can split it, although that actually results in a somewhat inconsistent commit (as it has dependencies no one needs, only one commit later) 🤔
but I get the idea from the standpoint of reviewing…

Also do we actually need to put the vendored deps into the git repo? We removed all vendoring from "images" because it is not packaged, maybe we could look into doing the same here?

Don't know - we can follow up on this, vendoring makes things strange for sure 👌

lzap

Perfect.

cmd/image-builder-maintenance/main.go

templates/maintenance.yml

croissanne

Thank you!

In addition to the comments, could you also clean up the commit messages a bit?

The body of the commit should be full sentences ( so capital letters & periods). There are also some spelling errors (depentent -> dependent, our selfs -> ourselves). And there's a trailing s in the distribution/Dockerfile-ubi-maintenance: fix entrypoint commit.

internal/tutils/db_tutils.go

cmd/image-builder-maintenance/db.go

Makefile

cmd/image-builder-maintenance/config.go

internal/tutils/db_tutils.go

cmd/image-builder-maintenance/config.go

build_deploy.sh

cmd/image-builder-maintenance/main.go

cmd/image-builder-maintenance/db.go

croissanne · 2024-11-25T15:28:40Z

@mvo5

Also do we actually need to put the vendored deps into the git repo? We removed all vendoring from "images" because it is not packaged, maybe we could look into doing the same here?

Don't know - we can follow up on this, vendoring makes things strange for sure 👌

I don't think there's any really good reason at this point time. We can remove them in image builder, a separate PR though.

mvo5 · 2024-11-25T15:34:16Z

[..]

In addition to the comments, could you also clean up the commit messages a bit?

Quick drive-by (as I was tagged): maybe also worth squashing a few commits

schuellerf · 2024-11-25T15:53:24Z

[..]

Quick drive-by (as I was tagged): maybe also worth squashing a few commits

This was on purpose to be able to follow along the comments easier, but I'll squash some.

Will be used in more database tests like the image-builder-maintenance.

image-builder-maintenance for now runs vacuum and cleans all composes older than 2 years (by default). To remove customer related data according to the retention policy. As this is not really urgent, it's scheduled once a week on Tuesdays to avoid problems on weekends. Also introduces a github action test if the container can still be built.

This avoids problems with snyk code checker. The password is set in the environment anyway, so this is not needed here.

This touches also other code, due to dependent functions.

The time is just a first starting point. Two hours should be sufficient.

Graceful shutdown on SIGTERM and SIGINT, even when currently executing SQL statements.

Clones are implicitly deleted by `ON DELETE CASCADE`.

Adds the `image-builder-maintenance` binary to the existing container `quay.io/cloudservices/image-builder`, to avoid duplicating all the necessary build jobs.

croissanne

lgtm!

cmd/image-builder-maintenance/config.go

schuellerf force-pushed the HMS-4244-implement-image-builder-database-maintenance branch from 8e097b5 to 12c937d Compare November 14, 2024 12:58

schuellerf force-pushed the HMS-4244-implement-image-builder-database-maintenance branch from 12c937d to 0e26d6e Compare November 14, 2024 13:06

schuellerf requested a review from croissanne November 14, 2024 16:42

schuellerf force-pushed the HMS-4244-implement-image-builder-database-maintenance branch from 6af80e6 to dc5cafe Compare November 15, 2024 10:50

lzap previously approved these changes Nov 15, 2024

View reviewed changes

schuellerf dismissed lzap’s stale review via 56e65d8 November 18, 2024 14:22

schuellerf force-pushed the HMS-4244-implement-image-builder-database-maintenance branch 6 times, most recently from e5de212 to e75755c Compare November 19, 2024 09:45

schuellerf requested a review from lzap November 19, 2024 10:29

schuellerf enabled auto-merge (rebase) November 19, 2024 10:29

lzap previously approved these changes Nov 20, 2024

View reviewed changes

cmd/image-builder-maintenance/db.go Show resolved Hide resolved

templates/maintenance.yml Show resolved Hide resolved

cmd/image-builder-maintenance/main.go Show resolved Hide resolved

schuellerf force-pushed the HMS-4244-implement-image-builder-database-maintenance branch from 6ffb370 to 326712c Compare November 21, 2024 13:24

mvo5 reviewed Nov 21, 2024

View reviewed changes

schuellerf dismissed lzap’s stale review via 86af5db November 22, 2024 09:43

schuellerf force-pushed the HMS-4244-implement-image-builder-database-maintenance branch 3 times, most recently from eb6caf2 to ef9f5b7 Compare November 22, 2024 10:10

schuellerf requested a review from mvo5 November 22, 2024 10:11

schuellerf requested a review from lzap November 22, 2024 10:11

lzap previously approved these changes Nov 25, 2024

View reviewed changes

cmd/image-builder-maintenance/main.go Show resolved Hide resolved

templates/maintenance.yml Show resolved Hide resolved

schuellerf dismissed lzap’s stale review via e07fe65 November 25, 2024 13:33

schuellerf force-pushed the HMS-4244-implement-image-builder-database-maintenance branch from e07fe65 to 7dce7d5 Compare November 25, 2024 13:34

schuellerf requested a review from lzap November 25, 2024 13:50

croissanne requested changes Nov 25, 2024

View reviewed changes

croissanne reviewed Nov 25, 2024

View reviewed changes

cmd/image-builder-maintenance/db.go Show resolved Hide resolved

mvo5 mentioned this pull request Nov 25, 2024

vendor: remove the vendored dependencies #1418

Open

schuellerf force-pushed the HMS-4244-implement-image-builder-database-maintenance branch 2 times, most recently from 83b2fb2 to d7185ec Compare November 25, 2024 17:47

schuellerf requested a review from croissanne November 25, 2024 17:47

schuellerf force-pushed the HMS-4244-implement-image-builder-database-maintenance branch from d7185ec to 6297665 Compare November 26, 2024 10:26

schuellerf added 9 commits November 26, 2024 16:55

image-builder-db-test: extract functions to tutils

7a5e717

Will be used in more database tests like the image-builder-maintenance.

internal/tutils/db_tutils: remove password from tests

aacce0d

This avoids problems with snyk code checker. The password is set in the environment anyway, so this is not needed here.

image-builder-maintenance: introduce context

7316a32

This touches also other code, due to dependent functions.

Makefile: extend make push-check with ubi-maintenance-container-test

18b4330

image-builder-maintenance/main: introduce timeout for maintenance

1911e4a

The time is just a first starting point. Two hours should be sufficient.

image-builder-maintenance: implement graceful shutdown on signal

3f5b611

Graceful shutdown on SIGTERM and SIGINT, even when currently executing SQL statements.

image-builder-maintenance/db: remove explicit deletion of clones

3dda36b

Clones are implicitly deleted by `ON DELETE CASCADE`.

templates/maintenance.yml: use the existing container for maintenance

1722b09

Adds the `image-builder-maintenance` binary to the existing container `quay.io/cloudservices/image-builder`, to avoid duplicating all the necessary build jobs.

schuellerf force-pushed the HMS-4244-implement-image-builder-database-maintenance branch from 02b4375 to 1722b09 Compare November 26, 2024 15:55

croissanne approved these changes Nov 27, 2024

View reviewed changes

cmd/image-builder-maintenance/config.go Show resolved Hide resolved

croissanne disabled auto-merge November 27, 2024 16:27

croissanne merged commit 4ecc626 into osbuild:main Nov 27, 2024
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement image builder database maintenance (HMS-4244) #1402

Implement image builder database maintenance (HMS-4244) #1402

schuellerf commented Nov 14, 2024 •

edited

Loading

codecov bot commented Nov 14, 2024 •

edited

Loading

schuellerf commented Nov 15, 2024

lzap left a comment •

edited

Loading

schuellerf commented Nov 15, 2024 •

edited

Loading

schuellerf commented Nov 15, 2024

schuellerf commented Nov 19, 2024 •

edited

Loading

schuellerf commented Nov 19, 2024

schuellerf commented Nov 19, 2024

lzap left a comment

mvo5 left a comment

schuellerf commented Nov 22, 2024

lzap left a comment

croissanne left a comment

croissanne commented Nov 25, 2024

mvo5 commented Nov 25, 2024

schuellerf commented Nov 25, 2024

croissanne left a comment

Implement image builder database maintenance (HMS-4244) #1402

Implement image builder database maintenance (HMS-4244) #1402

Conversation

schuellerf commented Nov 14, 2024 • edited Loading

codecov bot commented Nov 14, 2024 • edited Loading

Codecov Report

schuellerf commented Nov 15, 2024

lzap left a comment • edited Loading

Choose a reason for hiding this comment

schuellerf commented Nov 15, 2024 • edited Loading

schuellerf commented Nov 15, 2024

schuellerf commented Nov 19, 2024 • edited Loading

schuellerf commented Nov 19, 2024

schuellerf commented Nov 19, 2024

lzap left a comment

Choose a reason for hiding this comment

mvo5 left a comment

Choose a reason for hiding this comment

schuellerf commented Nov 22, 2024

lzap left a comment

Choose a reason for hiding this comment

croissanne left a comment

Choose a reason for hiding this comment

croissanne commented Nov 25, 2024

mvo5 commented Nov 25, 2024

schuellerf commented Nov 25, 2024

croissanne left a comment

Choose a reason for hiding this comment

schuellerf commented Nov 14, 2024 •

edited

Loading

codecov bot commented Nov 14, 2024 •

edited

Loading

lzap left a comment •

edited

Loading

schuellerf commented Nov 15, 2024 •

edited

Loading

schuellerf commented Nov 19, 2024 •

edited

Loading